Oh also obviously BLATANTLY MISSING: TESTS. ANY TESTS.

Any tips on testing this kind of thing would be appreciated. My local experiments have mostly consisted of asserting that serializing and deserializing round-trips correctly.

Currently grinding through the serde test suite, lots of little derive bugs.

Starting to regret changing a bunch of things from -> Body to -> (Option<Common>, Body, Option<FromBody>) in order to prevent duplicating a static.

Pushed fixes:

• Fixed bad cfg's in builtin impl commit
• Got existing test-suite passing with derive changes

One thing I didn't realize until now is that deserialize_from kinda doesn't make any sense when doing "remote" impls?

I have fallen behind on PRs so I haven't had a chance to review the implementation yet, but -- I would be interested in exploring some ways to modularize the serde and serde_derive crates better so that large but uncommonly required features like this one can be delivered in their own crate. The serde_state / serde_derive_state feature is in a similar situation where it seems somewhat overwhelming to maintain as part of Serde but the community that wants the feature is willing to maintain it on the side.

Maybe this means factoring a lot of the serde_derive code into a separate pre-1.0 helper crate that is depended on by serde_derive, serde_derive_state, and a deserialize_from derive crate.

Would you be interested in figuring out a structure that would work for deserialize_from as part of getting this integrated? Otherwise would it be possible to provide deserialize_from in its own crate somehow, even if it means some code duplication with serde_derive?

I don't really see how deserialize_from could be seperated out, realisitically? It's part of the trait definition.

I don't really see how deserialize_from could be seperated out, realisitically? It's part of the trait definition.

You can make a DeserializeFrom trait which is implemented for all Deserialize types

How would that work? It can't be blanketed, because actual type-specific code needs to be emitted.

Okay I can see how shipping this outside of the Deserialize trait definition would be problematic.

I am on board with accepting this as long as it does not affect compile times for anyone not using deserialize_from (basically the same concern as for clone_from). As is, when I tried this it raises compile time for the serde crate by 13% (5.9 seconds to 6.7 seconds) which is a bit much for something that few people will use. Worse, Deserialize impls already compile very slow (serde-rs/json#313) so something like 2x more generated code in every impl would not be nice.

Could you propose what you think is the best way to feature gate this?

Up to you how you want to get this merged. If it is easier, we can merge the serde pieces up front and follow up in one or more other PRs with the serde_derive pieces as those are written and tested.

One thing I don't like here is all the deserialize_from impls that are conceptually no different from the default implementation. For example the bool one:

fn visit_bool<E>(self, v: bool) -> Result<(), E> {
    *self.0 = v;
    Ok(())
}

Could you share some benchmarks that show these outperform the default deserialize_from? Is there an easy way to tell which types benefit from a custom deserialize_from? Or is a custom deserialize_from always preferable?

Ok finally got a chance to double check primitive stuff. Looks like, at least on webrender, removing the primitive impls is neutral, or possibly even profitable (the deserializer went from 7.1% to 6.6% of backend time; probably noise).

I'll take them out, then.

Ok I refactored out all the deserialize_from derivation code out so it could be feature'd out. Ultimately we weren't sharing a ton of code, so it's not a huge loss. Writing it that way helped me incrementalize the development process for myself better, but that doesn't matter now.

I also did a kinda naive addition of deserialize_from to the test suite.

Webrender, and therefore gecko, will likely be moving forward with a soft fork of serde in the next few days with these patches, plus the additional patch found on https://github.com/Gankro/serde/tree/deserialize_from_enums3

Even if you aren't ready to make a call on merging this, we'd certainly appreciate more thorough review. :)

I'd be on board with merging this PR as it is if we make the deserialize_from feature require nightly. Then we can play around with it a little without having to immediately stabilize it

That makes sense to me.

Wait, would that also put the trait method behind the feature?

Also CC #855

Hmm right, that could be messy with derives and such :/

I started making progress on the review -- pushed a few fixup commits as I go. Some remaining items:

• Before merging, I would like to get a sense of what this means for the future of csv::Reader and serde_json::StreamDeserializer. Is there a nice way to adapt these APIs to take advantage of deserialize_from? (I have not started thinking about this; it may turn out to be an easy answer.)

• Benchmarks! It seems obvious that this would be faster in some usage patterns, but I would want to see at least one basic not-too-contrived benchmark that I can run using a real existing data format to see an improvement from this.

• Currently for every struct with named fields we emit two totally identical copies of the __Field enum and the __FieldVisitor and its Visitor and Deserialize impls, which is a ton of redundant code. These should be emitted in a higher scope and shared by deserialize and deserialize_from. It is not critical to fix this before merging; will file an issue to follow up.

1. I don't know what's interesting about those, I'll take a look in a bit.
2. I'll try to cut out some basic workloads. Where do you put benches?
3. Yes, I originally put in a fair bit of work trying to avoid this, but I backtracked on it when I deintegrated stuff to support cfg(). This ends up being pretty nasty to thread through the code for things like deserializing struct-like enum variants. Not clear it's worth the effort.

The CSV reader and JSON stream deserializer are interesting because they are APIs designed for deserializing lots of instances of the same type that are processed one at a time -- seems like an obvious candidate for deserialize_from. CSV is why we filed #855 in the first place so we may as well be certain that we are addressing their use case in the best way.

There aren't really benchmarks of Serde by itself since nothing interesting happens without a data format, but JSON benchmarks live here and Bincode benchmarks live here. I wouldn't bother with either of those, not looking for anything fancy just a 30 line thing I can run and see an improvement. I would do this myself but I am about to call it a night. 😴